Add support for optional Kafka properties from external file #8743

klDen · 2021-08-01T13:54:02Z

kokosing

Tests are missing. Can you please add a case where configuration from resource file is used and we can see the changed behavior from tests?

docs/src/main/sphinx/connector/kafka.rst

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaConfig.java

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/SslKafkaAdminFactory.java

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/PlainTextKafkaProducerFactory.java

kokosing

We need to come up with a testing, to make sure that it is actually used in all places? Do you have any idea how it can be tested?

docs/src/main/sphinx/connector/kafka.rst

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaConfig.java

plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaConfig.java

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/DefaultTextKafkaProducerFactory.java

klDen · 2021-08-06T14:20:29Z

We need to come up with a testing, to make sure that it is actually used in all places? Do you have any idea how it can be tested?

Hello @kokosing ! Yes, I'm currently in the process of implementing a new product test and let the tests utilize the new config.

kokosing · 2021-08-06T15:17:55Z

I does have to be a product test. Maybe an unit test?

charlesjmorgan

It would be nice to have tests that use actual values in the external resource configs so that we can understand how they interact with the different security protocols and trino config options.

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaSecurityConfig.java

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/PlainTextKafkaConsumerFactory.java

docs/src/main/sphinx/connector/kafka.rst

klDen · 2021-08-08T00:01:03Z

I does have to be a product test. Maybe an unit test?

I was thinking of having a product test that runs purely on DefaultTextConfig. I'm open for suggestions to implement tests differently! I agree that adding unnecessarily a new product test can increase overall build time.

If we want to go forward with Unit tests, perhaps I can add 2 new tests in TestKafkaPlugin:

No security protocol defined in kafka.properties, but defined in resource.config.properties;
kafka.security-protocol=PLAINTEXT defined in kafka.properties with unmanaged Kafka configs by Trino in resource.config.properties.

WDYT @kokosing ?

gsvlad · 2021-11-08T11:31:39Z

Hi! In our project we use Confluent and want to produce from Trino to its topics. Confluent supports SASL method of authentication, so we are interested in this PR. Are there any blockers for it be released?

matt12eagles

These are working for me! Very cool klDen!

matt12eagles · 2021-12-20T15:07:55Z

@kokosing any chance you can get this approved and push it in?? :) would love to have this in the official builds. Thanks!

matt12eagles · 2022-02-22T14:50:55Z

We are waiting for this as well... Any update here team?

JackPott · 2022-02-25T14:26:35Z

I note there are a huge number of open PRs, how long do reviews and merges usually take?

kokosing

Please rebase. It is looks nice. Just few comments.

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaConfig.java

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/PlainTextKafkaConsumerFactory.java

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/utils/ConfigurationUtils.java

matt12eagles · 2022-03-09T18:54:37Z

Hi All! Any updates here?? I'm not exactly sure who we are waiting for at this point. Thanks team!

klDen · 2022-03-10T20:21:10Z

Hi All! Any updates here?? I'm not exactly sure who we are waiting for at this point. Thanks team!

Hey. I'll resolve the remaining comments this Sunday.

Praveen2112 · 2022-03-11T06:42:49Z

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaClientsModule.java

@@ -38,19 +40,30 @@ private void installClientModule(SecurityProtocol securityProtocol, Module modul
    {
        install(conditionalModule(
                KafkaSecurityConfig.class,
-                config -> config.getSecurityProtocol().equals(securityProtocol),
+                config -> config.getSecurityProtocol().orElse(null) == securityProtocol,


Can we use Optional#filter(SecurityProtocol::equals).isPresent()

Applying this fails TestKafkaPlugin tests since securityProtocol is nullable and calling security::equals throws NullPointerException.

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaConfig.java

Praveen2112 · 2022-03-11T06:44:40Z

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaSecurityConfig.java

@@ -27,12 +28,11 @@

 public class KafkaSecurityConfig
 {
-    private SecurityProtocol securityProtocol = PLAINTEXT;
+    private SecurityProtocol securityProtocol;


Can we have here as Optional<SecurityProtocol> ?

Setting it as Optional<SecurityProtocol> breaks the unit test since it expects an Optional.empty and actual is null.

public class TestKafkaSecurityConfig { @Test public void testDefaults() { assertRecordedDefaults(recordDefaults(KafkaSecurityConfig.class) .setSecurityProtocol(null));

The signature public KafkaSecurityConfig setSecurityProtocol(SecurityProtocol securityProtocol) accepts a plain SecurityProtocol object and not an Optional<SecurityProtocol> one.

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/PlainTextKafkaAdminFactory.java

klDen · 2022-03-16T22:09:18Z

The failing ci maven-checks is unrelated to my changes:

2022-03-16T06:45:25.760Z	ERROR	main	io.trino.server.Server	io.trino.spi.Plugin: Provider io.trino.plugin.deltalake.DeltaLakePlugin not found
java.util.ServiceConfigurationError: io.trino.spi.Plugin: Provider io.trino.plugin.deltalake.DeltaLakePlugin not found
	at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:589)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.nextProviderClass(ServiceLoader.java:1212)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1221)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1265)
	at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1300)
	at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1385)
	at com.google.common.collect.ImmutableList.copyOf(ImmutableList.java:275)
	at com.google.common.collect.ImmutableList.copyOf(ImmutableList.java:239)
	at io.trino.server.PluginManager.loadPlugin(PluginManager.java:166)
	at io.trino.server.PluginManager.loadPlugin(PluginManager.java:157)
	at io.trino.server.ServerPluginsProvider.lambda$loadPlugins$1(ServerPluginsProvider.java:58)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at com.google.common.util.concurrent.DirectExecutor.execute(DirectExecutor.java:31)
	at java.base/java.util.concurrent.ExecutorCompletionService.submit(ExecutorCompletionService.java:184)
	at io.trino.util.Executors.executeUntilFailure(Executors.java:41)
	at io.trino.server.ServerPluginsProvider.loadPlugins(ServerPluginsProvider.java:53)
	at io.trino.server.PluginManager.loadPlugins(PluginManager.java:137)
	at io.trino.server.Server.doStart(Server.java:126)
	at io.trino.server.Server.lambda$start$0(Server.java:80)
	at io.trino.$gen.Trino_c116fb3____20220316_064516_1.run(Unknown Source)
	at io.trino.server.Server.start(Server.java:80)
	at io.trino.server.TrinoServer.main(TrinoServer.java:38)


🚨 Failed to execute a query after Trino container started
4d094b0ad9bc0319bdf797e2e0c6557378b8abc3d65fc6fee44f031c64fb3c20
Error: Process completed with exit code 1.

kokosing

Please rebase and squash commits. There is also last comment from me.

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaConfig.java

klDen · 2022-03-31T07:36:31Z

Please rebase and squash commits. There is also last comment from me.

Squashed!

kokosing · 2022-04-03T20:57:12Z

@Praveen2112 or @lukasz-walkiewicz do you want to take a look?

kokosing · 2022-04-11T14:29:25Z

Merged, thanks!

hashhar · 2022-08-10T07:30:06Z

The external file doesn't support referencing secrets since it's not passed through via Bootstrap probably.

Not sure if this can be implemented however.

cla-bot bot added the cla-signed label Aug 1, 2021

klDen requested a review from kokosing August 1, 2021 13:54

kokosing reviewed Aug 2, 2021

View reviewed changes

klDen force-pushed the feature/kafka-config branch 5 times, most recently from 100e9a8 to ee0927e Compare August 4, 2021 03:43

kokosing reviewed Aug 6, 2021

View reviewed changes

charlesjmorgan reviewed Aug 6, 2021

View reviewed changes

klDen force-pushed the feature/kafka-config branch from ee0927e to 4ad54c1 Compare August 7, 2021 23:57

klDen force-pushed the feature/kafka-config branch 3 times, most recently from 5c6d725 to b49cac1 Compare August 8, 2021 00:12

klDen requested a review from kokosing August 8, 2021 00:41

klDen force-pushed the feature/kafka-config branch 2 times, most recently from 4c48b79 to 057628b Compare August 8, 2021 02:23

klDen requested a review from charlesjmorgan August 8, 2021 02:25

klDen force-pushed the feature/kafka-config branch 3 times, most recently from 13df6f8 to 8911101 Compare August 11, 2021 02:31

klDen force-pushed the feature/kafka-config branch 3 times, most recently from 3f26668 to 12793f4 Compare December 1, 2021 05:48

matt12eagles approved these changes Dec 10, 2021

View reviewed changes

kokosing reviewed Feb 28, 2022

View reviewed changes

Praveen2112 reviewed Mar 11, 2022

View reviewed changes

klDen force-pushed the feature/kafka-config branch 4 times, most recently from 34bb045 to fde302c Compare March 14, 2022 06:38

klDen requested a review from kokosing March 14, 2022 07:02

klDen force-pushed the feature/kafka-config branch 2 times, most recently from 0eca91c to 9bb94b3 Compare March 15, 2022 06:47

klDen requested a review from Praveen2112 March 15, 2022 06:49

klDen force-pushed the feature/kafka-config branch from 9bb94b3 to 0a37bb9 Compare March 16, 2022 03:20

kokosing approved these changes Mar 18, 2022

View reviewed changes

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaConfig.java Show resolved Hide resolved

Add support for optional Kafka properties from external file

be4b572

klDen force-pushed the feature/kafka-config branch from 0a37bb9 to be4b572 Compare March 31, 2022 07:35

kokosing merged commit 910501b into trinodb:master Apr 11, 2022

kokosing mentioned this pull request Apr 11, 2022

Release notes for 377 #11863

Closed

github-actions bot added this to the 377 milestone Apr 11, 2022

mosabua mentioned this pull request Apr 11, 2022

Add Trino 377 release notes #11859

Merged

klDen deleted the feature/kafka-config branch August 10, 2022 13:44

whatsupbros mentioned this pull request Aug 19, 2022

Add support to Kafka connector for connecting to secured Schema Registry #12195

Open

MisLink mentioned this pull request Dec 2, 2022

How about client.id config for kafka connector? #7633

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for optional Kafka properties from external file #8743

Add support for optional Kafka properties from external file #8743

klDen commented Aug 1, 2021 •

edited

Loading

kokosing left a comment

kokosing left a comment

klDen commented Aug 6, 2021

kokosing commented Aug 6, 2021

charlesjmorgan left a comment

klDen commented Aug 8, 2021 •

edited

Loading

gsvlad commented Nov 8, 2021

matt12eagles left a comment

matt12eagles commented Dec 20, 2021

matt12eagles commented Feb 22, 2022

JackPott commented Feb 25, 2022

kokosing left a comment

matt12eagles commented Mar 9, 2022

klDen commented Mar 10, 2022

Praveen2112 Mar 11, 2022

klDen Mar 14, 2022

Praveen2112 Mar 11, 2022

klDen Mar 14, 2022 •

edited

Loading

klDen commented Mar 16, 2022

kokosing left a comment

klDen commented Mar 31, 2022

kokosing commented Apr 3, 2022

kokosing commented Apr 11, 2022

hashhar commented Aug 10, 2022

Add support for optional Kafka properties from external file #8743

Add support for optional Kafka properties from external file #8743

Conversation

klDen commented Aug 1, 2021 • edited Loading

kokosing left a comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

klDen commented Aug 6, 2021

kokosing commented Aug 6, 2021

charlesjmorgan left a comment

Choose a reason for hiding this comment

klDen commented Aug 8, 2021 • edited Loading

gsvlad commented Nov 8, 2021

matt12eagles left a comment

Choose a reason for hiding this comment

matt12eagles commented Dec 20, 2021

matt12eagles commented Feb 22, 2022

JackPott commented Feb 25, 2022

kokosing left a comment

Choose a reason for hiding this comment

matt12eagles commented Mar 9, 2022

klDen commented Mar 10, 2022

Praveen2112 Mar 11, 2022

Choose a reason for hiding this comment

klDen Mar 14, 2022

Choose a reason for hiding this comment

Praveen2112 Mar 11, 2022

Choose a reason for hiding this comment

klDen Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

klDen commented Mar 16, 2022

kokosing left a comment

Choose a reason for hiding this comment

klDen commented Mar 31, 2022

kokosing commented Apr 3, 2022

kokosing commented Apr 11, 2022

hashhar commented Aug 10, 2022

klDen commented Aug 1, 2021 •

edited

Loading

klDen commented Aug 8, 2021 •

edited

Loading

klDen Mar 14, 2022 •

edited

Loading